added support to display fingerings as annotations#285
added support to display fingerings as annotations#285it-ony wants to merge 1 commit intostringsync:masterfrom
Conversation
jaredjj3
left a comment
jaredjj3
left a comment
There was a problem hiding this comment.
Would you go to tests/integration/w3c-musicxml.test.ts
uncomment this line:
// { filename: 'fingering-element-notation.musicxml', width: 900 },then run:
npm run test -- --updateSnapshotto update the snapshots for fingering annotations?
If you run into any issues, check out https://github.com/stringsync/vexml?tab=readme-ov-file#development. If you don't have Docker or the time, let me know and I can do it after you merge.
src/parsing/musicxml/annotation.ts
Outdated
|
|
||
| static fromFingering(config: Config, log: Logger, musicXML: { fingering: musicxml.Fingering }): Annotation | undefined { | ||
| const number = musicXML.fingering.getNumber(); | ||
| if (!number) { |
There was a problem hiding this comment.
I don't know if finger 0 is valid (I guess semantics are up to the transcriber), but would you make this check slightly more robust checking presence with typeof number !== 'number'?
There was a problem hiding this comment.
Makes sense. I think my linter changed it to the falsely notation. Will change it if course.
There was a problem hiding this comment.
I went with if (number === null || number === undefined) { at the end as I've seen not everything coming from the xml is actual a number.
Therefore, I'd say
export class Fingering {
constructor(private element: NamedElement<'fingering'>) {}
/** Returns the fingering number. Defaults to null. */
getNumber(): number | null {
return this.element.content().int();
}
}
needs adjustment as well to support all the optional attributes and not force conversion to int.
Checking the xsd
<xs:complexType name="fingering">
<xs:annotation>
<xs:documentation>Fingering is typically indicated 1,2,3,4,5. Multiple fingerings may be given, typically to substitute fingerings in the middle of a note. The substitution and alternate values are "no" if the attribute is not present. For guitar and other fretted instruments, the fingering element represents the fretting finger; the pluck element represents the plucking finger.</xs:documentation>
</xs:annotation>
<xs:simpleContent>
<xs:extension base="xs:string">
<xs:attribute name="substitution" type="yes-no"/>
<xs:attribute name="alternate" type="yes-no"/>
<xs:attributeGroup ref="print-style"/>
<xs:attributeGroup ref="placement"/>
</xs:extension>
</xs:simpleContent>
</xs:complexType>
example of a note I found.
<note default-x="79.07" default-y="-145.00">
<pitch>
<step>C</step>
<octave>3</octave>
</pitch>
<duration>4</duration>
<voice>5</voice>
<type>half</type>
<stem>down</stem>
<staff>2</staff>
<notations>
<technical>
<fingering>C</fingering>
</technical>
</notations>
</note>
There was a problem hiding this comment.
Nice find, I was just thinking selfishly as a guitarist!
I think the right call is to update the signature of Fingering.getNumber to return a string | null (or string with empty string as the default).
vexml/src/musicxml/fingering.ts
Lines 14 to 16 in 429f317
Let me know if you have other ideas.
@it-ony I deeply appreciate the contribution, you're pretty spot on! I'll write a |
I struggled most to understand the differences/responsibilities for the similar types in the project. I'm right now in my mobile that's why I can only tell from my head, but note is read from xml, then there is a class representation, and I think I found two more note classes or types. When rendered with vexflow (also here I took a while to understand the difference between vexml and vexflow) the click listener returned an object referencing a render note, which has a reference to a vexflow note, with similar but different properties. If would be nice if the differences and responsibility could be pointed out. |
Appreciate the feedback! Each directory has a README.md, but they're very lackluster. I'll improve it and probably centralize all this information, too. |
This is brilliant! It was me not spending time to actual look for this kind of documentation. I really enjoyed understanding the types for score/parts/voice etc... As I'm not a musician. Just building my first music product to learn piano better. I found the readme in the root already stunning, but the ones with the directories with their goals/ no goals that's next level. I should adapt this for my projects. |
it-ony
force-pushed
the
feat/fingering-annotations
branch
from
f3adb0c to
a907a5f
Compare
|
I had an emergency. I'll take a look at this PR and add documentation for contributors soon. |
Thanks! I'm glad you like them. I started writing directory-scoped intent docs to try to bridge the gap between humans and AI coding agents. Since then, the agents.md "standard" emerged and I felt like that did a much better job than what I was trying to do. I tried a different idea that decouples intents (aka specs) from the file structure of a project: https://github.com/stringsync/spec. It's a lightweight version of the Kiro IDE or https://github.com/github/spec-kit. Maybe you'll find one of those projects inspiring or useful! |
Done: https://github.com/stringsync/vexml?tab=contributing-ov-file#readme |
2 participants
Added support to show fingerings as top annotations.
Ideally annotations would have a type so the renderer in vexflow could introduce a config SHOW_FINGERING to only display the finger numbers, when configured.
But as this is my first contribution to the project and I didn't find a
CONTRIBUTION.mdso that's my best guess.